Fix disabled endpoints for vhosts not being reflected correctly #1190
Conversation
DDH13
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
WalkthroughThe changes introduce helper functions for intelligent endpoint and host selection across the DevPortal and Publisher applications. The DevPortal refactors AsyncApiConsole to prefer WebSocket or HTTP endpoints based on API type, while the Publisher's environment and deployment configuration pages add comprehensive WebSocket/host validation to prevent invalid selections and guide user interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/DeploymentOnbording.jsx (1)
232-237: Default vhost selection can pick WS‑disabled vhosts for WebSocket APIsFor WS APIs,
defaultVhostsalways usese.vhosts[0]viagetHostValue(e.vhosts[0], api.isWebSocket()). If the first vhost has WS endpoints disabled while a later vhost has them enabled,hasValidHosts(row)will still be true (because some vhost is valid), but the default selection for that env will point at the wrong vhost and be used increateDeployRevision.To avoid deploying a WS API to a vhost where WS is disabled, base the default host on the first vhost that actually has valid WS endpoints when
api.isWebSocket()is true.- const defaultVhosts = selectedInternalGateways.map((e) => { - if (e.vhosts && e.vhosts.length > 0) { - return { - env: e.name, - vhost: getHostValue(e.vhosts[0], api.isWebSocket()), - }; - } else { - return undefined; - } - }); + const defaultVhosts = selectedInternalGateways.map((e) => { + if (!e.vhosts || e.vhosts.length === 0) { + return undefined; + } + const candidates = api.isWebSocket() + ? e.vhosts.filter((v) => hasValidWebSocketPorts(v)) + : e.vhosts; + const first = candidates[0]; + return first + ? { env: e.name, vhost: getHostValue(first, api.isWebSocket()) } + : undefined; + }); @@ - const defaultVhosts = selectedExternalGateways.map((e) => { - if (e.vhosts && e.vhosts.length > 0) { - return { - env: e.name, - vhost: getHostValue(e.vhosts[0], api.isWebSocket()), - }; - } else { - return undefined; - } - }); + const defaultVhosts = selectedExternalGateways.map((e) => { + if (!e.vhosts || e.vhosts.length === 0) { + return undefined; + } + const candidates = api.isWebSocket() + ? e.vhosts.filter((v) => hasValidWebSocketPorts(v)) + : e.vhosts; + const first = candidates[0]; + return first + ? { env: e.name, vhost: getHostValue(first, api.isWebSocket()) } + : undefined; + });Also applies to: 253-258
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx (1)
608-619: Default vhost selection for WS APIs can still pick a vhost with WS disabledAs in
DeploymentOnbording.jsx,defaultVhostshere always usese.vhosts[0]and wraps it withgetHostValue(e.vhosts[0], api.isWebSocket()). For WS APIs, if the first vhost has WS endpoints disabled but a later vhost has them enabled,hasValidHosts(e)will be true, yet the default selection will target the WS‑disabled vhost and be used in deployments.This undermines the intent of the new validity checks.
Refine the default selection to prefer a vhost with valid WS endpoints for WS APIs:
- const defaultVhosts = selectedInternalGateways.map((e) => { - if (e.vhosts && e.vhosts.length > 0) { - return { - env: e.name, - vhost: getHostValue(e.vhosts[0], api.isWebSocket()) - }; - } else { - return undefined; - } - }); + const defaultVhosts = selectedInternalGateways.map((e) => { + if (!e.vhosts || e.vhosts.length === 0) { + return undefined; + } + const candidates = api.isWebSocket() + ? e.vhosts.filter((v) => hasValidWebSocketPorts(v)) + : e.vhosts; + const first = candidates[0]; + return first + ? { env: e.name, vhost: getHostValue(first, api.isWebSocket()) } + : undefined; + }); @@ - const defaultVhosts = selectedExternalGateways.map((e) => { - if (e.vhosts && e.vhosts.length > 0) { - return { - env: e.name, - vhost: getHostValue(e.vhosts[0], api.isWebSocket()) - }; - } else { - return undefined; - } - }); + const defaultVhosts = selectedExternalGateways.map((e) => { + if (!e.vhosts || e.vhosts.length === 0) { + return undefined; + } + const candidates = api.isWebSocket() + ? e.vhosts.filter((v) => hasValidWebSocketPorts(v)) + : e.vhosts; + const first = candidates[0]; + return first + ? { env: e.name, vhost: getHostValue(first, api.isWebSocket()) } + : undefined; + });This keeps default behavior unchanged for HTTP APIs, while ensuring WS APIs default to a vhost where WS is actually enabled.
Also applies to: 631-642
🧹 Nitpick comments (5)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiUI.jsx (2)
57-62: Disabled styling looks fine; consider using theme palette for consistencyThe
.Mui-disabledstyling on the Select matches the intended UX and is scoped correctly to.selectList. If you want this to stay in sync with the app theme, you could optionally refactorRootto take({ theme })and usetheme.palette.action.disabledBackground/theme.palette.text.disabledinstead of hard‑coded colors, but it’s not required for this PR.
78-90: Centralized endpoint selection is good; double‑check preference for insecure vs secure URLs
getPreferredEndpointnicely centralizes the initial endpoint selection and theuseEffectkeepsendPointin sync whenURLsorapi.typechange, which is a clear improvement.One thing to verify: the helper prefers non‑secure schemes when both are present:
- For WS APIs, it picks
wsfirst and only falls back towss.- For non‑WS APIs, it picks
httpfirst and only falls back tohttps.If both secure and insecure endpoints are configured, this will always pick the insecure one. That may be intentional (e.g., for local/dev usage), but if the desired behavior is to prefer secure endpoints when available, you may want to invert the order:
- if (apiType === CONSTANTS.API_TYPES.WS) { - // prefer ws, but if ws does not exist fall back to wss - if (URLsObj.ws) return URLsObj.ws; - if (URLsObj.wss) return URLsObj.wss; + if (apiType === CONSTANTS.API_TYPES.WS) { + // Prefer secure wss, fall back to ws if wss is not available + if (URLsObj.wss) return URLsObj.wss; + if (URLsObj.ws) return URLsObj.ws; } - // for non-WS APIs prefer http, but if http does not exist fall back to https - if (URLsObj.http) return URLsObj.http; - if (URLsObj.https) return URLsObj.https; + // For non-WS APIs prefer https, fall back to http if https is not available + if (URLsObj.https) return URLsObj.https; + if (URLsObj.http) return URLsObj.http;Please confirm which ordering matches the product expectation before changing.
Also applies to: 92-92, 103-105
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/DeploymentOnbording.jsx (1)
175-195: Shared WS/host helper logic looks good, but consider centralizingThe wsDisabled/wssDisabled/hasValidWebSocketPorts/hasValidHosts/getHostValue helpers are clean and match those in
Environments.jsx, which is good for consistency. Given they are duplicated across components, consider extracting them into a shared utility to avoid drift on future changes.portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx (2)
550-574: WS/host helper set is consistent and readableThe wsDisabled/wssDisabled/hasValidWebSocketPorts/hasValidHosts/getHostValue helpers cleanly encapsulate WS vs HTTP host/port validity and selection, and they align with the same logic reused in
DeploymentOnbording.jsx.Consider moving these helpers to a shared module so
Environments.jsxandDeploymentOnbording.jsxcan import them from a single place.
2551-2555: getVhostHelperText and tooltips now communicate host availability clearlyThe WS branch of
getVhostHelperTextcorrectly resolves the selected vhost viawsHost/wssHostand returns a clear “No valid hosts available for this environment” message when WS endpoints are disabled on that vhost. Combined with the updated tooltips that fall back to that message when!hasValidHosts(row), this should make host availability much more transparent to users.Also applies to: 2568-2574, 3567-3575, 3795-3803
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiUI.jsx(4 hunks)portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx(10 hunks)portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/DeploymentOnbording.jsx(7 hunks)portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiUI.jsx (1)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiConsole.jsx (1)
URLs(91-91)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/DeploymentOnbording.jsx (1)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx (5)
wsDisabled(550-550)wssDisabled(551-551)hasValidWebSocketPorts(553-555)hasValidHosts(557-563)getHostValue(566-574)
🔇 Additional comments (5)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiUI.jsx (1)
245-245: Guard against falsyURLswhen usingObject.keys/Object.valuesThe disable logic assumes
URLsis always a non‑null object:disabled={Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean)} {Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean) ? ( ... )}If
URLstransitions tonull/undefinedduring async environment loading,Object.keys()andObject.values()will throw. Add a falsy check:-disabled={Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean)} +disabled={!URLs || Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean)}Additionally, if
URLscan contain non-string values (e.g., nested objects or metadata), the MenuItems will render as[object Object]. Filter to string values only:Object.entries(URLs) .filter(([, value]) => typeof value === 'string' && value) .map(([key, value]) => ( <MenuItem value={value} key={key}>{value}</MenuItem> ));Applies to lines 245 and 248–259.
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/DeploymentOnbording.jsx (1)
459-460: Host validity checks and vhost dropdown filtering are aligned with WS semanticsDisabling environment selection (
Checkbox) and the vhostTextFieldwhen!hasValidHosts(row), and filtering vhosts for WS APIs viahasValidWebSocketPorts(vhost)before mapping toMenuItems, correctly prevents choosing vhosts with WS endpoints disabled. UsinggetHostValuefor both key and value keeps the stored value consistent for HTTP vs WS cases.Also applies to: 550-565, 684-686, 748-762
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx (2)
121-130: pickFirstEnabledUrl helper is simple and robustThe helper defensively handles missing/invalid
urlsand cleanly picks the first non-empty string, which keeps the call sites concise.
502-518: selectedEndpoint propTypes shape is now explicit and matches usageThe new
selectedEndpointshape (environmentName/Type plus nestedURLsanddefaultVersionURLs) matches how the component reads these fields and should help catch mismatches at runtime.portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx (1)
795-803: Guard in handleChange correctly prevents selecting environments without valid hostsThe early return when
event.target.checked && environment && !hasValidHosts(environment)prevents adding environments that have no valid hosts for the current API type intoSelectedEnvironment, which complements the UIdisabledstates. This is a good safety net around the selection logic.
| const getDefaultVersionUrl = () => { | ||
| const { defaultVersionURLs } = selectedEndpoint; | ||
| if (defaultVersionURLs | ||
| && (defaultVersionURLs.https | ||
| || defaultVersionURLs.http | ||
| || defaultVersionURLs.ws | ||
| || defaultVersionURLs.wss)) { | ||
| const firstEnabledDefaultUrl = pickFirstEnabledUrl(defaultVersionURLs); | ||
| if (firstEnabledDefaultUrl) { | ||
| return ( | ||
| <> | ||
| {` | ||
| ${intl.formatMessage({ | ||
| id: 'Apis.Details.Environments.default.url', | ||
| defaultMessage: '( Default Version ) ', | ||
| })} | ||
| ${(defaultVersionURLs.https || defaultVersionURLs.http || defaultVersionURLs.ws || defaultVersionURLs.wss)}`} | ||
| ${(firstEnabledDefaultUrl)}`} | ||
| <Tooltip |
There was a problem hiding this comment.
HTTP and GraphQL WebSocket URLs both using the same first URL is incorrect
pickFirstEnabledUrl(selectedEndpoint.URLs) is used for:
- The main “Gateway URL” (HTTP) display and copy.
- The “Gateway Websocket URL for GraphQL Subscriptions” display and copy.
This means the GraphQL WS section can end up showing/copying the HTTP URL instead of the ws/wss URL (or vice versa), depending on object property order.
HTTP and WS sections should select independently:
- HTTP: prefer
httpsthenhttp. - WS: prefer
wssthenws.
One way to fix this is to derive separate URLs before rendering:
- const getDefaultVersionUrl = () => {
- const { defaultVersionURLs } = selectedEndpoint;
- const firstEnabledDefaultUrl = pickFirstEnabledUrl(defaultVersionURLs);
+ const getDefaultVersionUrl = () => {
+ const { defaultVersionURLs } = selectedEndpoint;
+ const firstEnabledDefaultUrl = pickFirstEnabledUrl(defaultVersionURLs);
@@
- ${(firstEnabledDefaultUrl)}`}
+ ${firstEnabledDefaultUrl}`}
@@
- navigator.clipboard.writeText(firstEnabledDefaultUrl).then(onCopy);
+ navigator.clipboard.writeText(firstEnabledDefaultUrl).then(onCopy);And for the main HTTP and GraphQL WS URLs:
- <InputBase
+ {/*
+ Prefer HTTPS/HTTP for the main gateway URL
+ */}
+ <InputBase
@@
- value={pickFirstEnabledUrl(selectedEndpoint.URLs)}
+ value={pickFirstEnabledUrl({
+ https: selectedEndpoint.URLs.https,
+ http: selectedEndpoint.URLs.http,
+ })}
@@
- navigator.clipboard.writeText(pickFirstEnabledUrl(selectedEndpoint.URLs)).then(onCopy);
+ const httpUrl = pickFirstEnabledUrl({
+ https: selectedEndpoint.URLs.https,
+ http: selectedEndpoint.URLs.http,
+ });
+ navigator.clipboard.writeText(httpUrl).then(onCopy);
@@
- <InputBase
+ {/*
+ Prefer WSS/WS for GraphQL subscription URL
+ */}
+ <InputBase
@@
- value={pickFirstEnabledUrl(selectedEndpoint.URLs)}
+ value={pickFirstEnabledUrl({
+ wss: selectedEndpoint.URLs.wss,
+ ws: selectedEndpoint.URLs.ws,
+ })}
@@
- navigator.clipboard.writeText(pickFirstEnabledUrl(
- selectedEndpoint.URLs,
- )).then(onCopy);
+ const wsUrl = pickFirstEnabledUrl({
+ wss: selectedEndpoint.URLs.wss,
+ ws: selectedEndpoint.URLs.ws,
+ });
+ navigator.clipboard.writeText(wsUrl).then(onCopy);This keeps the “first enabled” behavior while ensuring each section uses the correct protocol family.
Also applies to: 248-253, 272-277, 284-341, 327-331
🤖 Prompt for AI Agents
In
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx
around lines 140-152, the GraphQL WebSocket URL is being derived using the same
pickFirstEnabledUrl call as the HTTP Gateway URL which can return an HTTP URL
for the WS field; instead, derive two separate URLs before rendering: one HTTP
candidate (pickFirstEnabledUrl from selectedEndpoint.URLs filtered to prefer
https then http) and one WS candidate (pickFirstEnabledUrl from
selectedEndpoint.URLs filtered to prefer wss then ws). Replace usages that
currently share the single pickFirstEnabledUrl result with these two variables
so each section independently selects the correct protocol family; apply the
same change to the other affected ranges (248-253, 272-277, 284-341, 327-331).
| if (!wsDisabled(vhost)) { | ||
| endpoints.primary = 'ws://' + vhost.wsHost + ':' + vhost.wsPort; | ||
| } | ||
| if (!wssDisabled(vhost)) { | ||
| endpoints.secondary = | ||
| 'wss://' + vhost.wssHost + ':' + vhost.wssPort; | ||
| } |
There was a problem hiding this comment.
Vhost dropdowns should consistently hide WS‑disabled vhosts for WebSocket APIs
You now:
- Build WS URLs in
getGatewayAccessUrlonly when WS/WSS isn’t disabled. - Use
hasValidHosts(row)to disable many controls and show a clear “No valid hosts…” tooltip. - Use
getHostValueto store/display the correct host for HTTP vs WS.
However, in the “Gateway Access URL / Select Access URL” dropdowns in the gateway tables you still map over all row.vhosts without filtering for WS validity, even for WS APIs:
// Internal gateways table
row.vhosts.map((vhost) => (
<MenuItem key={getHostValue(vhost, api.isWebSocket())}
value={getHostValue(vhost, api.isWebSocket())}>
{getHostValue(vhost, api.isWebSocket())}
</MenuItem>
));
// External gateways table – same pattern
row.vhosts.map((vhost) => ( ... ));For WS APIs this can expose vhosts where WS endpoints are disabled (leading to empty WS URLs), which contradicts the stricter handling you have elsewhere (e.g., in DeploymentOnbording.jsx you already filter by hasValidWebSocketPorts).
Align these dropdowns with the rest of the WS behavior:
- {row.vhosts.map(
- (vhost) => (
- <MenuItem
- key={getHostValue(vhost, api.isWebSocket())}
- value={getHostValue(
- vhost, api.isWebSocket())}>
- {getHostValue(vhost, api.isWebSocket())}
- </MenuItem>
- ),
- )}
+ {row.vhosts
+ .filter((vhost) =>
+ !api.isWebSocket()
+ || hasValidWebSocketPorts(vhost)
+ )
+ .map((vhost) => {
+ const hostValue = getHostValue(
+ vhost,
+ api.isWebSocket(),
+ );
+ return (
+ <MenuItem
+ key={hostValue}
+ value={hostValue}
+ >
+ {hostValue}
+ </MenuItem>
+ );
+ })}
@@
- {row.vhosts.map(
- (vhost) => (
- <MenuItem
- key={getHostValue(vhost, api.isWebSocket())}
- value={getHostValue(
- vhost, api.isWebSocket())}>
- {getHostValue(vhost, api.isWebSocket())}
- </MenuItem>
- ),
- )}
+ {row.vhosts
+ .filter((vhost) =>
+ !api.isWebSocket()
+ || hasValidWebSocketPorts(vhost)
+ )
+ .map((vhost) => {
+ const hostValue = getHostValue(
+ vhost,
+ api.isWebSocket(),
+ );
+ return (
+ <MenuItem
+ key={hostValue}
+ value={hostValue}
+ >
+ {hostValue}
+ </MenuItem>
+ );
+ })}With this plus the default‑vhost fix above, WS APIs will only allow selecting vhosts where WS endpoints are actually available, and tooltips/disabled states will fully match behavior.
Also applies to: 2550-2555, 2570-2574, 2963-2965, 2989-3010, 3567-3576, 3600-3607, 3611-3618, 3795-3803, 3828-3832, 3837-3842
🤖 Prompt for AI Agents
In
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx
around lines 2141-2147 (and also update the other listed ranges), the Gateway
Access URL dropdowns currently map over row.vhosts unfiltered which exposes
vhosts with WS/WSS disabled for WebSocket APIs; when api.isWebSocket() is true,
filter row.vhosts to only include vhosts that have valid WebSocket ports (e.g.,
use hasValidWebSocketPorts(vhost) or the same
!wsDisabled(vhost)/!wssDisabled(vhost) checks used elsewhere) before mapping,
and continue to use getHostValue(vhost, api.isWebSocket()) for
key/value/display; apply this change to all mentioned line ranges so WS APIs
only show selectable vhosts that actually provide WS/WSS endpoints.
1 participant
Summary by CodeRabbit
Bug Fixes
UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.